navigator: fix acceptance radius for multicopter #17661
Conversation
|
|
||
| // We use the acceptance radius of the mission item if it has been set (not NAN) | ||
| // but only for multicopter. | ||
| if (_navigator->get_vstatus()->vehicle_type == vehicle_status_s::VEHICLE_TYPE_ROTARY_WING |
There was a problem hiding this comment.
Can't this logic live inside of get_acceptance_radius()? We could again pass _mission_item.acceptance_radius as an argument.
There was a problem hiding this comment.
I actually found the way it was before quite confusing. So get_acceptance_radius() had this overload but the overload was only used from this location, right?
There was a problem hiding this comment.
Right. If one anyway only wants to take the acceptance radius from the param then using get_default_acceptance_radius() would be cleaner though no? And always pass _mission_item.acceptance_radius as argument of get_acceptance_radius()?
|
3/4 SITL tests failed in "Fly VTOL mission". They all seem like they completed, but never disarmed.
Restarting to get more data. |
|
I'll see if I can reproduce. |
|
|
||
| // We use the acceptance radius of the mission item if it has been set (not NAN) | ||
| // but only for multicopter. | ||
| if (_navigator->get_vstatus()->vehicle_type == vehicle_status_s::VEHICLE_TYPE_ROTARY_WING |
There was a problem hiding this comment.
Right. If one anyway only wants to take the acceptance radius from the param then using get_default_acceptance_radius() would be cleaner though no? And always pass _mission_item.acceptance_radius as argument of get_acceptance_radius()?
| // We use the acceptance radius of the mission item if it has been set (not NAN) | ||
| // but only for multicopter. | ||
| if (_navigator->get_vstatus()->vehicle_type == vehicle_status_s::VEHICLE_TYPE_ROTARY_WING | ||
| && PX4_ISFINITE(_mission_item.acceptance_radius) && _mission_item.acceptance_radius > 0.0f) { |
There was a problem hiding this comment.
Is a comparison >0.0f safer than >FLT_EPSILON?
There was a problem hiding this comment.
Right, let me change that.
|
@julianoes SITL tests continue to fail - is this a release blocker? It is not a core safety issue as we default to the param in many other places, but some use cases might rely on setting the acceptance radius manually. |
|
@LorenzMeier it makes sense to get it in before the release and I think the CI error is unrelated:
|
julianoes
force-pushed
the
pr-acceptance-radius
branch
from
a8d90ab
to
cdaa4cb
Compare
This fixes a regression introduced in #16646 which meant that the acceptance radius was no longer used at all for multicopter, and instead only the NAV_ACC_RAD param was used. With this change we use the acceptance radius of the mission item again if it is actually set (and not NAN) which we did not do before, and we only do that for multicopter.
This seems to slip in e.g. as part of the VTOL_LAND command.
julianoes
force-pushed
the
pr-acceptance-radius
branch
from
cdaa4cb
to
27f4fc7
Compare
|
Only my suggestion |
This fixes a regression introduced in #16646 which meant that the acceptance radius was no longer used at all for multicopter, and instead only the NAV_ACC_RAD param was used.
With this change we use the acceptance radius of the mission item again if it is actually set (and not NAN) which we did not do before, and we only do that for multicopter.
Found while working on mavlink/MAVSDK#1443.